Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement engine_getBlobsV1 #9723

Merged
merged 9 commits into from
Sep 9, 2024
Merged

Conversation

michaelsproul
Copy link
Contributor

@michaelsproul michaelsproul commented Jul 23, 2024

This is a prototype implementation of engine_getBlobsV1, which allows the CL to fetch blobs from the EL to accelerate blob propagation:

Before merging there are a few things that need to be resolved:

  • Work out whether error handling is required on get_blobs or whether we should make it infallible (don't return a result).
  • Assess the performance impact of recomputing the versioned hashes during iteration (while holding the cache lock).
  • Determine a good location for the BlobAndProofV1 type.
  • Add tests.
  • Add to engine capabilities.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, only a few nits

use serde::{Deserialize, Serialize};

/// Blob type returned in responses to `engine_getBlobsV1`.
// FIXME(sproul): move to alloy?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but we can include it in this pr, but also add it to alloy and once we have a new release we can phase it out here

Comment on lines 67 to 68
/// The blob data.
pub blob: FixedBytes<BYTES_PER_BLOB>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 67 to 68
/// The blob data.
pub blob: FixedBytes<BYTES_PER_BLOB>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should definitely box this because we do vec![None; request.len()] so each entry even if missing will be a full blob

@@ -14,6 +14,7 @@ workspace = true
[dependencies]

# ethereum
alloy-eips = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
alloy-eips = { workspace = true }
alloy-eips.workspace

.inner
.tx_pool
.get_blobs_for_versioned_hashes(&versioned_hashes)
.expect("get_blobs_for_versioned_hashes is infallible"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can return a servererror in this case

@mattsse mattsse added the A-rpc Related to the RPC implementation label Jul 23, 2024
@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Aug 20, 2024
@github-actions github-actions bot closed this Aug 27, 2024
@mattsse mattsse reopened this Aug 27, 2024
@mattsse mattsse added M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity and removed S-stale This issue/PR is stale and will close with no further activity labels Aug 27, 2024
@mattsse
Copy link
Collaborator

mattsse commented Sep 9, 2024

@michaelsproul sorry about the conflicts,
this looks feature complete, I'd like to get this over the line, if you're okay with me fixing the conflicts?

@michaelsproul
Copy link
Contributor Author

thanks @mattsse that would be great

I started on it locally but got drawn into other things and will likely continue getting distracted

thanks for pushing it along!

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opening a few smaller followups for this, but this should work

@mattsse mattsse added this pull request to the merge queue Sep 9, 2024
Merged via the queue into paradigmxyz:main with commit d8b12ac Sep 9, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Related to the RPC implementation M-prevent-stale Prevents old inactive issues/PRs from being closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants